feat(graph_watchdog): param_drift detector (GRAPH_PARAM_DRIFT) - #580
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new param_drift graph_watchdog detector that detects ROS 2 node parameter drift (self-captured baseline and/or config-pinned expect values) and reports it as GRAPH_PARAM_DRIFT, with bounded, off-executor parameter-service reads and end-to-end config plumbing validation.
Changes:
- Implement
param_driftdetector (background reader thread + round-robin, budgeted parameter reads; aggregated WARN-level fault). - Add new unit/integration/e2e tests for policy logic, detector behavior, and nested config delivery through the real gateway.
- Update graph_watchdog documentation (README + design doc) and wire new tests into CMake.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/test_param_drift_policy.cpp | New unit tests for drift comparison + config parsing/flattening rules. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/test_param_drift_integration.cpp | New integration tests driving real parameter services and ReportFault behavior. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/e2e/test_config_plumbing_e2e.test.py | New launch_testing e2e coverage for nested config plumbing into detectors. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/src/detectors/param_drift_detector.cpp | New detector implementation (budgeted read loop + aggregation + prune/forget). |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/README.md | Documentation for param_drift keys/behavior and “Closing the loop” healing guidance. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/param_drift_policy.hpp | New ROS-free policy core and config parser/validator for param_drift. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/design/graph_watchdog.rst | Design documentation extended to include param_drift. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/CMakeLists.txt | Adds new gtest + integration + e2e targets for param_drift and config plumbing. |
Comments suppressed due to low confidence (2)
src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/e2e/test_config_plumbing_e2e.test.py:131
- This unittest.skipUnless scenario gating is a skipped test, which the project review guidelines disallow. Consider restructuring so only the scenario-relevant tests are discovered/executed for each CTest target (no skips).
class TestConfigPlumbingModeOff(unittest.TestCase):
"""Mode suppression: the nested `mode` config disables the detector."""
@unittest.skipUnless(SCENARIO == 'mode_off', 'runs only under the "mode_off" scenario launch')
def test_mode_off_suppresses_detector(self):
src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/e2e/test_config_plumbing_e2e.test.py:160
- unittest.skipUnless is used to gate the scenario-specific test case, but skipped tests are disallowed by the project's test rules. Restructure so each scenario target runs only its tests without skip decorators.
class TestConfigPlumbingModeOffYamlBool(unittest.TestCase):
"""A BARE `off` disables the detector, not just a quoted one."""
@unittest.skipUnless(
SCENARIO == 'mode_off_yaml_bool',
'runs only under the "mode_off_yaml_bool" scenario launch',
)
bburda
force-pushed
the
feat/graph-watchdog-param-drift
branch
from
July 29, 2026 16:52
48e3f77 to
df83df1
Compare
mfaferek93
reviewed
Jul 29, 2026
bburda
force-pushed
the
feat/graph-watchdog-param-drift
branch
3 times, most recently
from
July 31, 2026 12:34
68a273b to
8bf996e
Compare
Raises GRAPH_PARAM_DRIFT when a node parameter no longer matches its reference. The reference is either self-captured, the value the parameter had when the node armed, or pinned in config with expect. Only the pinned form catches a value that was already wrong at startup, because self-capture treats whatever it first sees as correct. The detector reports that a value moved and does not grade the consequence. Whether the new value breaks the robot is a question about the machine, not about the graph, so it raises at WARN. Unlike the detectors shipped so far, this one makes real service calls to other nodes instead of reading the local graph cache, so the sweep is bounded by max_reads_per_tick and walks the graph round-robin. That bounds the load on watched nodes and pays with coverage latency, in both directions: a drift is invisible until its node's turn comes, and so is a repair. The reads run on a thread the plugin owns, never on the gateway executor, so a node that stops answering stalls only this sweep. Also adds the config-plumbing e2e, three real gateway launches that prove nested per-detector config reaches a detector at all. Every C++ test calls configure() directly with hand-built JSON, so nothing else in the package covers the delivery path. The three launches cover the nested expect reader, mode: "off" as a string, and the bare mode: off that the ROS parser types as a YAML 1.1 boolean, which is the form operators write. Restores the healing-config section the README referred to twice without carrying, since it belongs with the first detector whose clear story depends on it. Raise the Pixi job timeout from 45 to 60 minutes. Its build step alone is about 32 minutes and the job has been finishing at 37-40 minutes for weeks, so it had no room left for a package that grows.
bburda
force-pushed
the
feat/graph-watchdog-param-drift
branch
from
July 31, 2026 15:59
8bf996e to
cdbb53b
Compare
mfaferek93
approved these changes
Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds the
param_driftdetector. It raisesGRAPH_PARAM_DRIFTwhen a node parameter no longer matches its reference, either the value self-captured when the node armed or a value pinned withexpect.Unlike the two detectors already here, this one makes service calls into other nodes instead of reading the local graph cache. The sweep is therefore bounded by
max_reads_per_tickand walks the graph round-robin, which bounds the load on watched nodes and pays with coverage latency in both directions: a drift is invisible until its node's turn comes, and so is a repair. The reads run on a thread the plugin owns, never on the gateway executor.It also brings the config-plumbing e2e, which is the first proof in this package that nested per-detector config reaches a detector at all.
The README referred twice to a "Closing the loop" section on healing configuration that was never carried over. It belongs with the first detector whose clear story depends on it, so it is restored here.
Issue
Type
Testing
26/26 green locally on Jazzy.
test_param_drift_policy: the pure comparison and config rules, glob matching, thebaselineandexpectcombinations.test_param_drift_integration: real nodes with real parameters over the real parameter service against a fakeReportFaultservice. Covers the round-robin budget, the grace-based prune when a node vanishes, the clear path, and a node that never answers, which must not hang the tick.test_config_plumbing_e2e: three real gateway launches. One proves the nestedexpectreader, onemode: "off"as a string, one the baremode: offthat the ROS parser types as a YAML 1.1 boolean, which is the form operators actually write.Checklist